fix(relay): close per-trade and per-order subscriptions on task exit - #365

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4817e4dfb9

Reviewed at 4817e4d, merged locally against current main (3641db9 — the branch is 9 commits behind and merges clean). On that merge: 294 Rust tests pass, cargo clippy --locked -- -D warnings and cargo check --locked --target wasm32-unknown-unknown — the exact CI commands — are clean.

The leak this closes is real, and I confirmed the cap it's based on. But as it stands the change replaces a working subscription with one that no relay accepts, so the mechanism it introduces never runs.

Overlap with #255 — and why I'd keep this one

#255 (feat(#182)) does the same two things in the same two functions: deterministic ids via subscribe_with_id, plus unsubscribe at each loop's exit. The two are mutually redundant and will conflict.

I'd continue with this PR, for reasons of substance rather than seniority:

• The diff. This is +60/−3. #255 is +497/−265, because a merge commit ran cargo fmt over the whole of orders.rs (main's copy has 1263 lines of rustfmt drift; the branch's has zero). The functional change there is ~70 lines; the rest is churn that would conflict with the 13 open PRs that touch orders.rs.
• The comments. #255 still carries three comment blocks that describe an ownership guard removed in an earlier round, and a doc-comment claiming "full trade pubkey hex, not a prefix" above a function that slices [..32].
• The series. This is part of the coordinated #348 Phase 2 work.

But #255 is ahead on one thing that matters here, and it has to be carried over before this merges — see below. I'd suggest closing #255 as superseded once this lands, with the 64-char finding (which came out of its review) and its length test brought over.

Blocking: mostro-daemon-<trade_pubkey_hex> is 78 characters; NIP-01 caps ids at 64

daemon_message_subscription_id builds "mostro-daemon-" + 64 hex = 78. SubscriptionId::new performs no length validation and subscribe_with_id returns Ok regardless, so nothing surfaces locally.

Probed against both entries of DEFAULT_RELAYS, same filter on three ids at once — this PR's, #255's 45-char id, and a short control:

[probe] pr365_len=78  pr255_len=45  ctrl_len=10
[probe] subscribe len=78 -> ok=true
[probe] CLOSED relay=wss://relay.mostro.network id_len=78 msg=Subscription id should be non-empty string of max length 64 chars
[probe] CLOSED relay=wss://nos.lol             id_len=78 msg=ERROR: bad req: invalid subscription id length
[probe] RESULT pr365(78)=0   pr255(45)=10   ctrl(10)=10

Zero events, on both relays, with independently worded errors — this is the NIP being enforced, not one relay's quirk.

It is a regression. On main the same subscription uses client.subscribe(filter, None), and nostr-sdk generates 16 bytes of hex — 32 characters, well inside the limit. So this change takes a subscription that works today and makes it one the relay refuses.

Why it isn't visible. Three layers hide it:

1. subscribe_with_id returns Ok — the REQ was sent; the refusal comes later.
2. The CLOSED arrives asynchronously and the relay-notification loop only logs it (blog_warn "relay" closed sub=… msg=…). Nothing propagates it, nothing retries.
3. The daemon's messages for that trade key still arrive over the global mostro-dm bulk feed, since ensure_global_dm_coverage includes the key. Create, take and restore keep working, so manual testing passes.

Meanwhile the watcher loops for its full 30-minute idle window over a subscription that was never established, and unsubscribes it on exit. The limit(0) live-only replay protection on that filter is also not in effect, because there is no subscription — correlation falls entirely to the bulk feed, which replays history. The request_id nonce still guards it, so no visible bug, but the property is gone.

Where the budget was lost. The pattern this mirrors — chat_subscription_id (messages.rs:889) — is "mostro-chat-" + "dispute-"? + a 36-char order id = 56 max. It fits because the key is an order id. Substituting a 64-char pubkey hex into the same shape crosses the limit.

The fix

Truncate: format!("mostro-daemon-{}", &trade_pubkey_hex[..32]) gives 46 characters. 128 bits rules out any realistic cross-trade collision, and it leaves headroom for a later prefix change.

Please also extend every_subscription_id_addresses_one_feed (or add a sibling) with a length invariant over both generators:

assert!(daemon_message_subscription_id(&a).to_string().len() <= 64);
assert!(single_order_subscription_id(order_id).to_string().len() <= 64);

The existing collision test is the right test to have — it's the one thing #255 lacks — but it passes happily on an id no relay will accept. Without the length assert, the next prefix change crosses the limit again and the bulk feed hides it again.

Blocking: the new doc-comment landed on the wrong function

The helpers were inserted above orders_subscription_id rather than below it, so /// Stable subscription ID for the Kind 38383 order-book feed. now sits directly on top of /// Stable id for a trade's daemon-message subscription. — daemon_message_subscription_id carries two contradictory doc lines, and orders_subscription_id is left with none. One line to move.

The failure mode in the description is not the one that happens

I measured the cap on relay.mostro.network (40 REQs on one connection, with a stand-in "order book" subscription opened first):

[probe] eose_count=19 closed_count=22
[probe] CLOSED probe-018: Number of subscriptions exceeds limit
[probe] CLOSED probe-019: Number of subscriptions exceeds limit
…
[probe] book_was_closed=false

The relay accepts ~18 concurrent subscriptions and then rejects the newcomer; it does not evict anything already open — the order-book stand-in stayed alive and got its EOSE. So the symptom isn't "an order list that quietly stops updating"; it's "past roughly 18 trade actions in a session, new per-trade subscriptions are silently refused" (and the client falls back to the bulk feed). The leak is worth closing either way, but the description overstates the damage and should say what actually happens.

Not blocking, worth recording

Two watchers can share mostro-order-<uuid>. The id is keyed by order id, not by trade key. take_order only refuses when order.status != Pending, so an order that returns to Pending (taker timeout, cancellation) and is taken again in the same session starts a second watcher on the same id. Harmless in the normal case — both read the same client.notifications() broadcast, reset their idle timers on the same events and exit together — except that subscribe_single_order's recv arm is Ok(Err(_)) => break, which treats a broadcast Lagged as fatal. A lagged older watcher would then tear down the younger one's REQ and leave it looping on nothing for up to 30 minutes. subscribe_daemon_messages gets this right (Lagged => continue, Closed => break). Matching the two arms is a one-line change; otherwise this belongs on #325.

Also verified

• Merge with current main is clean, one file, +60/−3.
• Unsubscribing doesn't blind the client: ensure_global_dm_coverage runs on all three paths that open a per-trade watcher (create_order, take_order, restore_session) and keeps every derived trade key in the bulk Kind-14 filter for the life of the process. That's what makes the teardown safe.
• One watcher per daemon-message id: all three callers call derive_trade_key() first, so the pubkey — and the id — is fresh each time. Unconditional cleanup is correct there.
• No privacy regression: the deterministic id shows the relay a trade pubkey / order id that the same subscription's filter already carries.
• Every loop exit reaches the cleanup; the only early return is the failed-subscribe path, where there is nothing to tear down.
• limit(0) is untouched — only the id changes.